const Vector VEC_HULL_MIN(-16, -16, -36);
const Vector VEC_DUCK_HULL_MIN(-16, -16, -18);
auto hull_offset = is_duck ? VEC_DUCK_HULL_MIN : VEC_HULL_MIN;

pev->origin[2] += hull_offset[2];

https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/util.h#L414-L424

So many repetition of this HULL values, maybe future refactor?

So many repetition of this HULL values, maybe future refactor?

Surely.

I know that now in the code these HULL values are declared several times in scope of different functions, but with a future refactor all this will be declared once in some header.

So let it be like this for now.

This is rebase of another other commit, maybe ill change it there instead

Rebase of another commit

Ugh. You know, clearing in SV_SpawnServer is generally not bad, because it is called when a new map starts, changelevels and similar events, and it is called before the engine calls DispatchKeyValue

ServerDeactivate (funcs.pfnServerDeactivate from DLL_FUNCTIONS table) is also called in those events and also before DispatchKeyValue, but the difference is that it also covers other events such as disconnect
At that point clearing in ServerDeactivate looks a bit safer, because for example if you are disconnected and you are in the menu, you know that the data will already be cleared, and in the case of SV_SpawnServer you need load map to clear this old data

So personally, I would advise hooking ServerDeactivate from the server-side (fortunately it doesn’t require patterns) and including a comment at the beginning of its function about the difference in data cleanup compared to SV_SpawnServer

You may also need to hook ServerActivate to track its state, because HLSDK does this to avoid unnecessary calls to ServerDeactivate, take a look at the highlighted lines: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/client.cpp#L685-L708
ServerActivate could be hooked from exported server table too, so not need to worries about patterns

What do you mean by tracking ServerActivate state? Tracking g_serveractive? So there needs to be either another pattern for that static value or BXT own static value for g_serveractive? Seems like a lot of extra work. I think clearing data on load is fine unless you are adamant about it then I can make some changes for that.

If you read the highlighted lines carefully, you will understand that you don’t need to intercept that g_serveractive variable at all. Let me show you a concept of how this can be done without patterns.

// Global variable
bool g_serveractive = false;

// Find "DLL_FUNCTIONS funcs;" in the server module for the declaration location
ORIG_ServerActivate = funcs.pfnServerActivate;
ORIG_ServerDeactivate = funcs.pfnServerDeactivate;

HOOK_DEF_0(ServerDLL, void, __cdecl, ServerDeactivate)
{
	// https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/client.cpp#L685-L708
	if (g_serveractive)
	{
		g_serveractive = false;

		// Peform any shutdown operations here...
	}

	ORIG_ServerDeactivate();
}

HOOK_DEF_3(ServerDLL, void, __cdecl, ServerActivate, edict_t*, pEdictList, int, edictCount, int, clientMax)
{
	g_serveractive = true;
	ORIG_ServerActivate(pEdictList, edictCount, clientMax);
}

on load is fine unless you are adamant about it then I can make some changes for that.

Well I'd fine with it in general because I don't think there would a cases when you want to access that data while being not on the server, and even so there seems a validation checks is done, so I'd like to say fine, but ServerDeactivate would be a nicer option in fact, maybe it should be done with future refactoring then

I totally understood what you mean. I just wonder if it is worth it. Do you think it is good to pursue this?

Github going slow. I agree it should be in the future refactor. This PR is another rebase of another PR so I am not very confident to make some changes here.

That's a lot of string comparisons that happen all the time (?), especially for an optional feature.

You made me paranoid so I debug print it. This is only called when a new server is started. Every time a new server is started, a lot of string comparisons like this happen to parse entities in BSP. This one is just one extra entity available in the game.